-
Notifications
You must be signed in to change notification settings - Fork 206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Build : Ensure gaffer
wrapper exists before calling usdGenSchema
#5838
Build : Ensure gaffer
wrapper exists before calling usdGenSchema
#5838
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching that Murray - made a suggestion inline that with a bit of luck might allow us to simplify things even further.
SConstruct
Outdated
@@ -1879,6 +1880,7 @@ for libraryName, libraryDef in libraries.items() : | |||
schemaSource, | |||
buildSchema | |||
) | |||
env.Depends( generatedSchema, "additionalFiles" ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also depend on the env
app and the Gaffer module being installed, neither of which are captured by additionalFiles
. But maybe we don't even need to use gaffer env
to run usdGenSchema
? It looks like commandEnv
already has LD_LIBRARY_PATH etc set, so maybe we can just call usdGenSchema
directly, without any extra dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! commandEnv
has almost everything we need, though it's missing $BUILD_DIR/python
in the PYTHONPATH
and without it usdGenSchema
can't find pxr
.
I pushed a fixup in cfd44d8 calling usdGenSchema
directly from a new env with pxr
on PYTHONPATH
, though maybe it's worth updating commandEnv
directly for any other future use-cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think you're right - we might as well get commandEnv
set up fully rather than make another env.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I've done the commandEnv
update and squashed it in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one, thanks!
3c4afd7
to
cfd44d8
Compare
This is a little simpler than using `gaffer env usdGenSchema` and solves an issue where the `gaffer` wrapper didn't exist when calling `gaffer env usdGenSchema` from a clean build.
cfd44d8
to
efd17ee
Compare
Noticed this while running a clean build where GafferArnold was being built at the same time. The Arnold schema was attempting to generate via
gaffer env usdGenSchema
before thegaffer
wrapper was installed.Not an issue on CI as we build Gaffer first and then the various GafferArnolds...